ZOOKEEPER-4391: Unable to use newer JVM memory settings as Xmx overrides - #2061
smoldenhauer-ish wants to merge 2 commits into
Conversation
5422711 to
22fa28e
Compare
22fa28e to
acf502c
Compare
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
I agree this is a problem, and I think this is an adequate workaround... but it forces somebody to set a dummy value in ZK_SERVER_HEAP in order to specify different memory options directly in SERVER_JVMFLAGS. Maybe an empty string is an adequate dummy value? In any case, this seems difficult to reason about.
I suggest this alternative: If the user set JVMFLAGS, just use that. Only use the HEAP variable if the user didn't set any JVMFLAGS.
Also, I suggest shipping with a default conf/java.env or conf/zookeeper-env.sh that contains these lines. The files in the bin/ directory shouldn't be considered user configuration, and should not do any environment setup that should be the responsibility of the user to customize. Those things should occur in the conf/ directory. See, for example: https://github.com/apache/accumulo/blob/rel/2.1.3/assemble/conf/accumulo-env.sh#L93-L100
| # default heap for zookeeper server | ||
| ZK_SERVER_HEAP="${ZK_SERVER_HEAP:-1000}" | ||
| export SERVER_JVMFLAGS="-Xmx${ZK_SERVER_HEAP}m $SERVER_JVMFLAGS" | ||
| if [[ ${ZK_SERVER_HEAP} =~ ^[0-9]+.*[0-9]+$ ]]; then |
There was a problem hiding this comment.
Curly braces aren't needed in these square brackets. Also, I'm not sure you have the regex correct. Yours would match on any of these clearly incorrect values:
00000 abc 000but not0
I think what you want is "a single non-zero digit, followed by any number of additional digits", which is ^[1-9][0-9]*$
| if [[ ${ZK_SERVER_HEAP} =~ ^[0-9]+.*[0-9]+$ ]]; then | |
| if [[ $ZK_SERVER_HEAP =~ ^[1-9][0-9]*$ ]]; then |
| if [[ ${ZK_SERVER_HEAP} =~ ^[0-9]+.*[0-9]+$ ]]; then | ||
| export SERVER_JVMFLAGS="-Xmx${ZK_SERVER_HEAP}m $SERVER_JVMFLAGS" | ||
| else | ||
| export SERVER_JVMFLAGS="${ZK_SERVER_HEAP} $SERVER_JVMFLAGS" |
There was a problem hiding this comment.
Curly braces aren't needed here:
| export SERVER_JVMFLAGS="${ZK_SERVER_HEAP} $SERVER_JVMFLAGS" | |
| export SERVER_JVMFLAGS="$ZK_SERVER_HEAP $SERVER_JVMFLAGS" |
| if [[ ${ZK_CLIENT_HEAP} =~ ^[0-9]+.*[0-9]+$ ]]; then | ||
| export CLIENT_JVMFLAGS="-Xmx${ZK_CLIENT_HEAP}m $CLIENT_JVMFLAGS" | ||
| else | ||
| export CLIENT_JVMFLAGS="${ZK_CLIENT_HEAP} $CLIENT_JVMFLAGS" | ||
| fi |
There was a problem hiding this comment.
An alternative approach that is much simpler:
| if [[ ${ZK_CLIENT_HEAP} =~ ^[0-9]+.*[0-9]+$ ]]; then | |
| export CLIENT_JVMFLAGS="-Xmx${ZK_CLIENT_HEAP}m $CLIENT_JVMFLAGS" | |
| else | |
| export CLIENT_JVMFLAGS="${ZK_CLIENT_HEAP} $CLIENT_JVMFLAGS" | |
| fi | |
| # set memory options only if the user didn't customize the CLIENT_JVMFLAGS | |
| if [[ -z $CLIENT_JVMFLAGS ]]; then | |
| export CLIENT_JVMFLAGS="-Xmx${ZK_CLIENT_HEAP}m" | |
| fi |
SERVER_JVMFLAGS are added to JVMFLAGS here: https://github.com/apache/zookeeper/blob/master/bin/zkServer.sh#L81 |
Yes, but that's not really relevant to the discussion. My suggestion is: if the user set |
|
Thanks for comments & suggestions. |
|
FYI, I proposed an alternative approach in #2460. |
4 participants
allows a more flexible usage of the ZK_SERVER_HEAP/ZK_CLIENT_HEAP environment variable to specify heap related JVM settings.
It also keeps the old setting of a megabyte number for -Xmx
ZK_SERVER_HEAP="1500" sets "-Xmx1500m"
ZK_SERVER_HEAP="-XX:+UseContainerSupport -XX:MaxRAMPercentage=75"
skips the -Xmxm and adds the ZK_SERVER_HEAP as specified to the SERVER_JVM_FLAGS
same for the client settings.